[AC-1454] Migrate 2fa.directory call by lizard-boy · Pull Request #71 · grepdemos/server

lizard-boy · 2024-10-19T09:09:56Z

Type of change

Objective

We currently call the 2fa.directory API every time a user generates the Inactive Two Factor report which isn't a very friendly approach. This PR now calls our backend instead. [Current settings] - we will cache the parsed results for 24 hours so we only hit the 2fa.directory API once/day.

Code changes

ReportsController.cs: Created Reports API controller with the future in mind of potentially moving around other report functions to the backend.
InactiveTwoFactorResponseModel.cs: Created response model containing a read only dictionary so the client doesn't have to do any further logic parsing in regards to the 2fa.directory data
GlobalSettings.cs: Added ITwoFactorDirectorySettings for potential customizations to the URL and cache expiration
ITwoFactorDirectorySettings.cs: Created settings object for use within GlobalSettings
TwoFactorDirectoryTotpResponseModel.cs: Created response model with desired fields for our flattened dictionary
GetInactiveTwoFactorQuery.cs: Created query to handle returning either a cached dictionary or a freshly parsed response from the 2fa.directory API. @r-tome created custom parser to handle the response json structure.
ToolsServiceCollectionExtensions.cs: Created collection extension to better organize future tools team specific services
ServiceCollectionExtensions.cs: Injected new tools services
test/*/GetInactiveTwoFactorQueryTests.cs: Created unit tests for the new query.

Shout-outs

Big thanks to @r-tome @jrmccannon @shane-melton and @MGibson1 for all the help/conversations throughout!

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

Greptile Summary

This pull request implements a caching mechanism for the inactive two-factor authentication report, reducing API calls to the 2fa.directory service.

Added ReportsController in /src/Api/Tools/Controllers/ReportsController.cs to handle caching and retrieval of 2FA report data
Introduced GetInactiveTwoFactorQuery in /src/Core/Tools/Queries/GetInactiveTwoFactorQuery.cs for fetching and caching 2FA directory data
Created TwoFactorDirectorySettings in /src/Core/Settings/GlobalSettings.cs to manage API URL and cache expiration
Added unit tests in /test/Core.Test/Tools/Queries/GetInactiveTwoFactorQueryTests.cs to verify caching and API interaction
Implemented feature flag MigrateTwoFactorDirectory in /src/Core/Constants.cs for controlled rollout of the new functionality

…safe, refs AC-1454

…AC-1454

…4-2fa-directory

…s AC-1454

…fs AC-1454

greptile-apps

_{12 file(s) reviewed, 13 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-10-19T09:10:39Z

+[Authorize("Application")]
+public class ReportsController : Controller
+{
+    private readonly ICurrentContext _currentContext;


style: _currentContext is declared but never used

@greptileai, can I remove this?

greptile-apps · 2024-10-19T09:10:39Z

+        var user = await _userService.GetUserByPrincipalAsync(User);
+        if (!user.Premium)
+        {
+            throw new UnauthorizedAccessException("Premium required");


style: Consider using a custom exception for premium requirements

greptile-apps · 2024-10-19T09:10:53Z

+{
+    public InactiveTwoFactorResponseModel() : base("inactive-two-factor") { }
+
+    public IReadOnlyDictionary<string, string> Services { get; set; }


style: Consider using init-only setter for immutability

greptile-apps · 2024-10-19T09:11:59Z

@@ -0,0 +1,7 @@
+namespace Bit.Core.Settings;
+
+public interface ITwoFactorDirectorySettings


style: Consider adding XML documentation comments to describe the interface's purpose

greptile-apps · 2024-10-19T09:12:15Z

+    [JsonPropertyName("documentation")]
+    public string Documentation { get; set; }
+    [JsonPropertyName("additional-domains")]
+    public IEnumerable<string> AdditionalDomains { get; set; }


style: Consider using IReadOnlyCollection instead of IEnumerable for AdditionalDomains to prevent accidental modifications

greptile-apps · 2024-10-19T09:12:37Z

+    private static IEnumerable<TwoFactorDirectoryTotpResponseModel> ParseTwoFactorDirectoryTotpResponse(string json)
+    {
+        var data = new List<TwoFactorDirectoryTotpResponseModel>();
+        using var jsonDocument = JsonDocument.Parse(json);
+        // JSON response object opens with Array notation
+        if (jsonDocument.RootElement.ValueKind == JsonValueKind.Array)
+        {
+            // Each nested array has two values: a floating "name" value [index: 0] and an object with desired data [index: 1]
+            data.AddRange(from element in jsonDocument.RootElement.EnumerateArray()
+                          where element.ValueKind == JsonValueKind.Array && element.GetArrayLength() == 2
+                          select element[1].Deserialize<TwoFactorDirectoryTotpResponseModel>());
+        }
+
+        return data;
+    }


style: Consider adding error handling for JSON parsing exceptions

greptile-apps · 2024-10-19T09:12:55Z

+
+public interface IGetInactiveTwoFactorQuery
+{
+    Task<Dictionary<string, string>> GetInactiveTwoFactorAsync();


style: Consider adding XML documentation comments to describe the method's purpose and return value.

greptile-apps · 2024-10-19T09:13:12Z

+    public static void AddToolsServices(this IServiceCollection services)
+    {
+        services.AddReportsQueries();
+    }


style: Consider adding XML documentation comments to describe the purpose of this public method

greptile-apps · 2024-10-19T09:13:32Z

+        sutProvider.GetDependency<IGlobalSettings>().TwoFactorDirectory.Returns(
+            new GlobalSettings.TwoFactorDirectorySettings()
+            {
+                CacheExpirationHours = 1,


logic: The cache expiration is set to 1 hour here, but the PR description mentions 24 hours. Ensure consistency across the implementation.

greptile-apps · 2024-10-19T09:13:32Z

+                Uri = new Uri("http://localhost")
+            });
+
+        await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.GetInactiveTwoFactorAsync());


logic: Throwing a BadRequestException for an Unauthorized response might not be the most appropriate exception. Consider using a more specific exception type.

greptile-apps

PR Summary

This pull request implements server-side caching for the 2fa.directory API calls, reducing load on the external service and improving response times for the inactive two-factor authentication report.

Added distributed caching in GetInactiveTwoFactorQuery.cs with configurable 24-hour expiration
Implemented premium user validation in ReportsController.cs to restrict access to paid accounts
Created custom JSON parser in GetInactiveTwoFactorQuery.cs to handle the specific 2fa.directory response format
Added comprehensive unit tests covering cache hits, API failures, and successful API responses
Protected endpoint with feature flag MigrateTwoFactorDirectory for controlled rollout

_{12 file(s) reviewed, 10 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-10-30T00:54:53Z

+
+    }


style: style: Remove extra blank line before closing brace

greptile-apps · 2024-10-30T00:55:09Z

+{
+    public InactiveTwoFactorResponseModel() : base("inactive-two-factor") { }
+
+    public IReadOnlyDictionary<string, string> Services { get; set; }


logic: Property allows write access despite being IReadOnlyDictionary. Consider making the setter private to ensure immutability.

greptile-apps · 2024-10-30T00:55:28Z

    /// flexible collections
    /// </summary>
    public const string FlexibleCollectionsMigration = "flexible-collections-migration";
+    public const string MigrateTwoFactorDirectory = "migrate-two-factor-directory";


style: Consider adding a documentation comment above this constant explaining its purpose, similar to other feature flags in this file

greptile-apps · 2024-10-30T00:55:50Z

+    public class TwoFactorDirectorySettings : ITwoFactorDirectorySettings
+    {
+        public Uri Uri { get; set; } = new("https://api.2fa.directory/v3/totp.json");
+        public int CacheExpirationHours { get; set; } = 24;


style: 24 hours may be too long for cache expiration - consider reducing to prevent stale data

greptile-apps · 2024-10-30T00:56:26Z

+    public Uri Uri { get; set; }
+    public int CacheExpirationHours { get; set; }


style: Consider making properties read-only to prevent unintended modifications after initialization

greptile-apps · 2024-10-30T00:56:46Z

+{
+    [Required]
+    [JsonPropertyName("domain")]
+    public string Domain { get; set; }


style: Domain property should be initialized to avoid null reference exceptions since it's required

greptile-apps · 2024-10-30T00:56:47Z

+    public string Domain { get; set; }
+    [JsonPropertyName("documentation")]
+    public string Documentation { get; set; }


style: Consider making properties init-only since they're only populated during deserialization

greptile-apps · 2024-10-30T00:57:07Z

+        using var client = _httpClientFactory.CreateClient();
+        var response =
+            await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, _globalSettings.TwoFactorDirectory.Uri));


style: Consider adding a timeout to the HTTP request to prevent hanging on slow responses

greptile-apps · 2024-10-30T00:58:04Z

+            .Returns(new HttpResponseMessage()
+            {
+                StatusCode = HttpStatusCode.OK,
+                Content = new StringContent("{}", Encoding.UTF8, MediaTypeNames.Application.Json)


style: style: Consider testing with actual 2FA directory response data instead of empty JSON to verify parsing logic

vincentsalucci and others added 16 commits July 11, 2023 13:56

feat: initial commit, refs AC-1454

700984e

Merge branch 'master' into ac1454-2fa-directory

54e2a20

feat: add unit tests and make two factor directory settings uri type-…

6a1607b

…safe, refs AC-1454

Merge branch 'master' into ac1454-2fa-directory

b45cd11

Merge branch 'master' into ac1454-2fa-directory

12bd746

fix: dotnet format, refs AC-1454

feebc50

Merge branch 'master' into ac1454-2fa-directory

6d5934c

Merge branch 'master' into ac1454-2fa-directory

54606c7

fix: remove TODO, only add Tools Services to API project scope, refs …

8d15389

…AC-1454

fix: removed unused statement, refs AC-1454

e3d7bf0

Merge branch 'master' into ac1454-2fa-directory

7b5ea1e

Merge branch 'main' of https://github.com/bitwarden/server into ac145…

e56925a

…4-2fa-directory

Merge branch 'main' into ac1454-2fa-directory

6bbccb3

feat: move tools team service init to bottom of services section, ref…

8a33050

…s AC-1454

Merge branch 'main' into ac1454-2fa-directory

c7e9f5f

feat: add MigrateTwoFactorDirectory feature flag / api protection, re…

2322789

…fs AC-1454

greptile-apps Bot reviewed Oct 19, 2024

View reviewed changes

greptile-apps Bot reviewed Oct 30, 2024

View reviewed changes

		@@ -0,0 +1,7 @@
		namespace Bit.Core.Settings;

		public interface ITwoFactorDirectorySettings

		public Uri Uri { get; set; }
		public int CacheExpirationHours { get; set; }

Uh oh!

Conversation

lizard-boy commented Oct 19, 2024 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Objective

Code changes

Shout-outs

Before you submit

Greptile Summary

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

dakshgup Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 19, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

greptile-apps Bot Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

lizard-boy commented Oct 19, 2024 •

edited by greptile-apps Bot

Loading